Skip to content

Check ls-tree Mode, Not Type, to Exclude a Symlink - #1020

Merged
ptr727 merged 2 commits into
developfrom
worktree-git-revisions-symlink-fix
Aug 26, 2026
Merged

Check ls-tree Mode, Not Type, to Exclude a Symlink#1020
ptr727 merged 2 commits into
developfrom
worktree-git-revisions-symlink-fix

Conversation

@ptr727

@ptr727 ptr727 commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Summary

A symlink's git ls-tree type is blob too, the same as a regular file: only its mode (120000) differs. Checking entry type alone let a file-to-symlink transition through as file content, and git show on a symlink revision returns the link's target path string, not the content it points to, so that string would be hashed and compared as if it were the file's real text.

Fix

Check the ls-tree mode field directly instead of the type: only 100644 (regular) and 100755 (executable) count as file content, everything else (040000 tree, 120000 symlink, 160000 gitlink, or absent) reads as None. Added a self-test covering a file-to-symlink transition.

Validation

  • python3 spec/audit.py --selftest
  • uvx ruff check / uvx ruff format --check spec/audit.py
  • uvx mypy spec/audit.py
  • python3 scripts/prose_lint.py (full check set)
  • python3 scripts/repo_gate.py

Raised by CodeRabbit on PR #1016 (develop -> main promotion).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of historical symlinks, directories, and submodules during audit comparisons.
    • Prevented unsupported path types from being treated as readable file content.
    • Added coverage for symlink transitions in audit validation.

A symlink's git ls-tree type is 'blob' too, the same as a regular
file: only its mode (120000) differs. Checking entry type alone let a
file-to-symlink transition through as file content, and git show on a
symlink revision returns the link's target path string, not the
content it points to, so that string would be hashed and compared as
if it were the file's real text.

Check the ls-tree mode field directly instead of the type: only
100644 (regular) and 100755 (executable) count as file content:
everything else (040000 tree, 120000 symlink, 160000 gitlink, or
absent) reads as None. Added a self-test covering a file-to-symlink
transition.

## Validation

- python3 spec/audit.py --selftest
- uvx ruff check / uvx ruff format --check spec/audit.py
- uvx mypy spec/audit.py
- python3 scripts/prose_lint.py (full check set)
- python3 scripts/repo_gate.py

Raised by CodeRabbit on PR #1016 (develop -> main promotion).
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Exclude Symlinks from Git Revision Content Using ls-tree Modes

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Identify regular revision content by mode, excluding symlinks and other non-files.
• Add regression coverage for file-to-symlink history transitions.
Diagram

graph TD
  A["Git history"] --> B["ls-tree entry"] --> C{"Regular mode?"}
  C -->|100644 or 100755| D["Read content"] --> E["Revision history"]
  C -->|other or absent| F["Record None"] --> E
Loading
High-Level Assessment

The explicit Git mode allowlist is the appropriate approach because object type cannot distinguish regular files from symlinks. It preserves executable-file support while safely treating trees, symlinks, gitlinks, and missing paths as contentless revisions.

Files changed (1) +57 / -9

Bug fix (1) +57 / -9
audit.pyExclude symlink revisions from comparable file content +57/-9

Exclude symlink revisions from comparable file content

• Changes '_git_revisions' to recognize file content only for Git modes '100644' and '100755', preventing symlink target strings from entering fidelity and staleness comparisons. Adds a temporary-repository self-test covering a regular-file-to-symlink transition and clarifies the mode-based behavior in documentation.

spec/audit.py

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8ef31846-c5b9-4a81-98b9-fb01b9e5d9f2

📥 Commits

Reviewing files that changed from the base of the PR and between 0064ec7 and b403bf4.

📒 Files selected for processing (1)
  • spec/audit.py

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

_git_revisions now reads historical content only from regular files with modes 100644 or 100755. Symlinks, directories, and submodules produce unavailable-content revisions. A self-test verifies file-to-symlink transitions.

Changes

Git revision handling

Layer / File(s) Summary
Regular-file validation and transition test
spec/audit.py
_git_revisions checks regular-file modes before reading historical content. Non-regular paths return None, and the self-test covers file-to-symlink transitions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to b403b

The change prevents symlink revisions from being treated as regular file content by checking Git modes directly. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: checking the Git tree mode instead of the entry type to exclude symlinks.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-git-revisions-symlink-fix

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

qodo-code-review Bot commented Aug 26, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (1)

Grey Divider


Remediation recommended

1. Mode comment wraps prose ✗ Dismissed 📜 Skill insight ⚙ Maintainability
Description
The new non-regular-entry comment is a five-line prose block whose two sentences are wrapped across
line boundaries. Comments must normally be one line and must place each complete sentence on its own
line.
Code

spec/audit.py[R1763-1766]

            # Absent (empty stdout), or present as something other than a regular file (a
-            # directory from a file-to-directory transition, a submodule gitlink): neither has
-            # file content to compare, so both read the same as a confirmed deletion.
+            # directory from a file-to-directory transition, a symlink, a submodule gitlink):
+            # none has file content to compare, so all read the same as a confirmed deletion. A
+            # symlink's ls-tree type is "blob" too (its content is the link target), so the mode
Relevance

●●● Strong

Recent accepted reviews explicitly require one sentence per line and concise prose comments in
spec/audit.py.

PR-#978
PR-#901

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rules 2826677 and 2826725 require short comments and prohibit sentences wrapped across comment
lines. The first sentence starts on line 1763 and ends on line 1765, while the second starts on line
1765 and ends on line 1767.

spec/audit.py[1763-1767]
Skill: comment-and-doc-style

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The comment above the `None` result spans five lines and wraps sentences in the middle.

## Issue Context
Retain only the concise reason the mode check is necessary, with one complete sentence per line and no more than two lines when a genuine constraint requires it.

## Fix Focus Areas
- spec/audit.py[1763-1767]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Symlink self-test can abort ✓ Resolved 🐞 Bug ☼ Reliability
Description
The new self-test calls Path.symlink_to() unconditionally, which raises when the host or user
lacks symlink-creation support or permission (commonly on Windows), aborting the entire --selftest
run before it can report results. This makes the repository's documented audit validation unusable
on otherwise supported development environments.
Code

spec/audit.py[3387]

+        (tmp_root_path / rel).symlink_to("target")
Relevance

●● Moderate

Windows portability findings are accepted, but no close precedent addresses symlink creation
capability in self-tests.

PR-#643

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The added line creates the symlink without a capability check or exception handler, and the
following staging, commit, and assertion logic assumes it succeeded. The audit's validation action
invokes spec/audit.py --selftest directly, so this exception escapes and terminates the whole
validation run.

spec/audit.py[3366-3408]
.github/actions/validate/action.yml[29-35]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `_git_revisions` self-test assumes symbolic-link creation is available. `Path.symlink_to()` can raise on hosts without symlink support or permission, aborting the full audit self-test suite.

## Issue Context
Keep testing the file-to-symlink transition where the platform supports it, but detect an unavailable symlink capability and skip this case cleanly rather than failing the entire self-test command.

## Fix Focus Areas
- spec/audit.py[3366-3408]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

3. Docstring exposes ls-tree internals ✗ Dismissed 📜 Skill insight ✧ Quality
Description
The added _git_revisions docstring text describes the internal ls-tree mode/type implementation
and git show behavior rather than only the function's behavior contract. This makes the contract
documentation dependent on implementation details.
Code

spec/audit.py[R1725-1728]

+    a regular file, by ls-tree mode rather than type (a directory from a file-to-directory
+    transition, a symlink, whose ls-tree type is "blob" too but whose content is its target path
+    rather than a file's, a submodule gitlink). A `git show` failure for any other reason (a
+    permission or encoding fluke, a corrupt object) raises instead of folding into the same None,
Relevance

●●● Strong

Recent accepted reviews enforce concise contract-focused docstrings and remove
implementation-specific prose.

PR-#978
PR-#901

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 2827096 requires docstrings to describe behavior contracts rather than
implementation details. The added text explicitly documents checking ls-tree mode rather than type
and explains what git show returns for symlinks.

spec/audit.py[1725-1730]
Skill: python-codestyle

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `_git_revisions` docstring includes internal Git command and mode/type implementation details instead of limiting itself to observable behavior.

## Issue Context
The contract only needs to state that revisions without regular-file content produce `None`. Implementation rationale can be moved to a concise inline comment near the mode check if necessary.

## Fix Focus Areas
- spec/audit.py[1725-1730]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Self-test comment cites PR 📜 Skill insight ✧ Quality
Description
The new self-test comment embeds the task-specific ptr727/ProjectTemplate#1016 reference and wraps
one sentence across three lines. PR context belongs in the PR description, while the code comment
should state only durable rationale in a concise sentence.
Code

spec/audit.py[R3366-3368]

+    # _git_revisions: a path that becomes a symlink reads as None too, per
+    # ptr727/ProjectTemplate#1016 (a symlink's ls-tree type is "blob", but git show returns its
+    # target path, not file content).
Relevance

● Weak

A recent review explicitly rejected removing task-specific PR references from docstrings and
self-test comments.

PR-#1004

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2827092 prohibits comments tied to the current task or PR, and rules 2826677 and 2826725
require concise comments without mid-sentence wrapping. Lines 3366-3368 contain an explicit PR
identifier and one sentence split across all three lines.

spec/audit.py[3366-3368]
Skill: python-codestyle
Skill: comment-and-doc-style

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The self-test comment references PR `ptr727/ProjectTemplate#1016` and wraps a single sentence across three lines.

## Issue Context
Remove the task reference and replace the block with a concise, durable explanation of the symlink behavior, using one complete sentence per line.

## Fix Focus Areas
- spec/audit.py[3366-3368]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 70 rules
✅ Skills: 5 invoked
  comment-and-doc-style
  dotnet-codestyle
  python-codestyle
  shell-codestyle
  workflow-ci-contract
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread spec/audit.py
Comment thread spec/audit.py
Comment thread spec/audit.py Outdated
Path.symlink_to() raises OSError on a host or user lacking
symlink-creation privilege, notably Windows without Developer Mode or
an elevated prompt. Calling it unconditionally in the self-test would
abort the whole --selftest run on such a host, before it could report
any result at all, over an environment limitation rather than a code
fault.

Wrap the symlink creation in try/except OSError and skip only that
one case with a printed note when it fails, leaving every other
self-test case (including the rest of _git_revisions' own coverage)
unaffected.

## Validation

- python3 spec/audit.py --selftest
- uvx ruff check / uvx ruff format --check spec/audit.py
- uvx mypy spec/audit.py
- python3 scripts/prose_lint.py (full check set)
- python3 scripts/repo_gate.py

Raised by qodo on PR #1020.
@ptr727
ptr727 merged commit 5ce0374 into develop Aug 26, 2026
8 checks passed
@ptr727
ptr727 deleted the worktree-git-revisions-symlink-fix branch August 26, 2026 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant